Skip to content

fix(QueryBuilder): Restrict identifier length to 30 characters due to Oracle limitations#48361

Merged
nickvergessen merged 1 commit intomasterfrom
fix/querybuilder/oracle-indentifier-length
Sep 26, 2024
Merged

fix(QueryBuilder): Restrict identifier length to 30 characters due to Oracle limitations#48361
nickvergessen merged 1 commit intomasterfrom
fix/querybuilder/oracle-indentifier-length

Conversation

@provokateurin
Copy link
Member

@provokateurin provokateurin commented Sep 26, 2024

Summary

According to https://docs.nextcloud.com/server/latest/admin_manual/installation/system_requirements.html we still support Oracle versions <12.2.
https://docs.oracle.com/en/database/oracle/oracle-database/23/sqlrf/Database-Object-Names-and-Qualifiers.html says:
If COMPATIBLE is set to a value of 12.2 or higher, then names must be from 1 to 128 bytes long and If COMPATIBLE is set to a value lower than 12.2, then names must be from 1 to 30 bytes long.
This means we must prevent any identifier with more than 30 characters as long as we support Oracle <12.2.

This check should also be extended to table names and schema names, but I'm not sure how to do that best.
I found this due to https://github.com/nextcloud/groupfolders/actions/runs/10960223441/job/30434816204 (which tests against Oracle 11) which triggers this query:

 SELECT `g`.`folder_id`,
       `g`.`group_id`,
       `g`.`circle_id`,
       `g`.`permissions`,
       `hp_cc`.`unique_id`                 AS `hp_cc_unique_id`,
       `hp_cc`.`name`                      AS `hp_cc_name`,
       `hp_cc`.`display_name`              AS `hp_cc_display_name`,
       `hp_cc`.`sanitized_name`            AS `hp_cc_sanitized_name`,
       `hp_cc`.`source`                    AS `hp_cc_source`,
       `hp_cc`.`description`               AS `hp_cc_description`,
       `hp_cc`.`settings`                  AS `hp_cc_settings`,
       `hp_cc`.`config`                    AS `hp_cc_config`,
       `hp_cc`.`contact_addressbook`       AS `hp_cc_contact_addressbook`,
       `hp_cc`.`contact_groupname`         AS `hp_cc_contact_groupname`,
       `hp_cc`.`creation`                  AS `hp_cc_creation`,
       `hp_cc_wn`.`circle_id`              AS `hp_cc_wn_circle_id`,
       `hp_cc_wn`.`member_id`              AS `hp_cc_wn_member_id`,
       `hp_cc_wn`.`single_id`              AS `hp_cc_wn_single_id`,
       `hp_cc_wn`.`user_id`                AS `hp_cc_wn_user_id`,
       `hp_cc_wn`.`instance`               AS `hp_cc_wn_instance`,
       `hp_cc_wn`.`user_type`              AS `hp_cc_wn_user_type`,
       `hp_cc_wn`.`level`                  AS `hp_cc_wn_level`,
       `hp_cc_wn`.`status`                 AS `hp_cc_wn_status`,
       `hp_cc_wn`.`note`                   AS `hp_cc_wn_note`,
       `hp_cc_wn`.`contact_id`             AS `hp_cc_wn_contact_id`,
       `hp_cc_wn`.`cached_name`            AS `hp_cc_wn_cached_name`,
       `hp_cc_wn`.`cached_update`          AS `hp_cc_wn_cached_update`,
       `hp_cc_wn`.`contact_meta`           AS `hp_cc_wn_contact_meta`,
       `hp_cc_wn`.`joined`                 AS `hp_cc_wn_joined`,
       `hp_cc_wn_on`.`unique_id`           AS `hp_cc_wn_on_unique_id`,
       `hp_cc_wn_on`.`name`                AS `hp_cc_wn_on_name`,
       `hp_cc_wn_on`.`display_name`        AS `hp_cc_wn_on_display_name`,
       `hp_cc_wn_on`.`sanitized_name`      AS `hp_cc_wn_on_sanitized_name`,
       `hp_cc_wn_on`.`source`              AS `hp_cc_wn_on_source`,
       `hp_cc_wn_on`.`description`         AS `hp_cc_wn_on_description`,
       `hp_cc_wn_on`.`settings`            AS `hp_cc_wn_on_settings`,
       `hp_cc_wn_on`.`config`              AS `hp_cc_wn_on_config`,
       `hp_cc_wn_on`.`contact_addressbook` AS `hp_cc_wn_on_contact_addressbook`,
       `hp_cc_wn_on`.`contact_groupname`   AS `hp_cc_wn_on_contact_groupname`,
       `hp_cc_wn_on`.`creation`            AS `hp_cc_wn_on_creation`
FROM   `*prefix*group_folders_groups` `g`
       LEFT JOIN `*prefix*circles_circle` `hp_cc`
              ON `hp_cc`.`unique_id` = `g`.`circle_id`
       LEFT JOIN `*prefix*circles_member` `hp_cc_wn`
              ON ( `hp_cc_wn`.`circle_id` = `hp_cc`.`unique_id` )
                 AND ( `hp_cc_wn`.`level` = :dcValue1 )
       LEFT JOIN `*prefix*circles_circle` `hp_cc_wn_on`
              ON `hp_cc_wn_on`.`unique_id` = `hp_cc_wn`.`single_id`

where hp_cc_wn_on_contact_addressbook is 31 characters.

Checklist

@provokateurin provokateurin added bug 3. to review Waiting for reviews labels Sep 26, 2024
@provokateurin provokateurin added this to the Nextcloud 31 milestone Sep 26, 2024
@provokateurin
Copy link
Member Author

provokateurin commented Sep 26, 2024

/backport to stable30

@provokateurin
Copy link
Member Author

provokateurin commented Sep 26, 2024

/backport to stable29

@provokateurin
Copy link
Member Author

provokateurin commented Sep 26, 2024

/backport to stable28

This was referenced Sep 26, 2024
Closed
Merged
come-nc
come-nc reviewed Sep 26, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not fix the bug but ensures it crashes on all DB, right?
Is there no way to magically fix the query instead, ideally in a class specific to OCI? I see we have Adapter, ExpressionBuilder and FuncitonBuilder specifc to OCI.

If we keep this as-is, what would the fix look like in the calling method?

@provokateurin
Copy link
Member Author

provokateurin commented Sep 26, 2024

This does not fix the bug but ensures it crashes on all DB, right?

It doesn't crash, it just logs an error. It will fail on Oracle when the query is executed eventually.

Is there no way to magically fix the query instead, ideally in a class specific to OCI? I see we have Adapter, ExpressionBuilder and FuncitonBuilder specifc to OCI.

I don't think we can magically fix this as the code receiving the results relies on the exact names.

If we keep this as-is, what would the fix look like in the calling method?

The aliases just need to be shorter. For example Circles adds a ton of prefixes to the name so I think for that case we could maybe drop some of them.
In theory we could still have a columns with names longer than 30 chars, but those need to be prevented at creation time.

come-nc
come-nc approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I missed that the Exception is not thrown but only logged.

@nickvergessen
Copy link
Member

nickvergessen commented Sep 26, 2024

This check should also be extended to table names and schema names, but I'm not sure how to do that best.

We do this already:

server/lib/private/DB/MigrationService.php

Lines 528 to 550 in 9836e9b

/**
* Naming constraints:
* - Tables names must be 30 chars or shorter (27 + oc_ prefix)
* - Column names must be 30 chars or shorter
* - Index names must be 30 chars or shorter
* - Sequence names must be 30 chars or shorter
* - Primary key names must be set or the table name 23 chars or shorter
*
* Data constraints:
* - Tables need a primary key (Not specific to Oracle, but required for performant clustering support)
* - Columns with "NotNull" can not have empty string as default value
* - Columns with "NotNull" can not have number 0 as default value
* - Columns with type "bool" (which is in fact integer of length 1) can not be "NotNull" as it can not store 0/false
* - Columns with type "string" can not be longer than 4.000 characters, use "text" instead
*
* @see https://github.com/nextcloud/documentation/blob/master/developer_manual/basics/storage/database.rst
*
* @param Schema $sourceSchema
* @param Schema $targetSchema
* @param int $prefixLength
* @throws \Doctrine\DBAL\Exception
*/
public function ensureOracleConstraints(Schema $sourceSchema, Schema $targetSchema, int $prefixLength) {

@nickvergessen nickvergessen merged commit 771f67f into master Sep 26, 2024
@nickvergessen nickvergessen deleted the fix/querybuilder/oracle-indentifier-length branch September 26, 2024 13:11
This was referenced Sep 26, 2024
Closed
Closed
Merged
@provokateurin provokateurin mentioned this pull request Oct 2, 2024
Merged
4 tasks
@dartcafe dartcafe mentioned this pull request Nov 4, 2024
Closed
12 tasks
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@come-nc come-nc come-nc approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug

Projects

None yet

Milestone

Nextcloud 31

Development

Successfully merging this pull request may close these issues.

3 participants

@provokateurin @nickvergessen @come-nc